fix(activesync): treat collection lock contention as transient during PING#75
Merged
Merged
Conversation
… PING A PING polling loop that hit a collection lock held by a parallel SYNC received a generic Horde_ActiveSync_Exception and reacted with the corrupt-state recovery path: a device state reset via loadState() with an empty collection array. This caused "Undefined array key" warnings in State/Base.php, and the reset itself failed again on the still-held lock, escaping as an uncaught exception and returning HTTP 500 to the client. In the worst case the recovery would wipe healthy sync state just because another request momentarily held the lock. Throw Horde_ActiveSync_Exception_TemporaryFailure on lock contention in the Sql and Mongo state drivers, and catch it in pollForChanges() to skip the affected collection until the next poll iteration instead of resetting state. Defer the StaleState recovery resets the same way when the lock is unavailable. Guard the folder object creation in loadState() against an empty collection array to silence the PHP 8 warnings on legitimate resets.
🔍 CI ResultsOverall: ❌ 12/12 lanes failed TL;DR: ❌ Quality issues: PHPUnit: 36 tests failed; PHPStan: 333 unique errors in 8 lanes. Summary by PHP Version
Quality Metrics
❌ Failed Lanesphp8.0-dev
php8.0-stable
php8.1-dev
php8.1-stable
php8.2-dev
php8.2-stable
php8.3-dev
php8.3-stable
php8.4-dev
php8.4-stable
php8.5-dev
php8.5-stable
CI powered by horde-components • View full results |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Horde_ActiveSync_Exception_TemporaryFailureinstead of a genericHorde_ActiveSync_Exception.loadState()no longer emits PHP 8 "Undefined array key" warnings when invoked with an empty collection array during error recovery.Motivation
Production logs showed this failure chain when a PING polled a collection whose lock was held by a parallel SYNC:
initCollectionState()threw the genericHorde_ActiveSync_Exception('Collection lock held.').pollForChanges()treated this as corrupt state and started a recovery reset:loadState([], null, SYNC, $id).Undefined array key "class"/"serverid"warnings inState/Base.php.Lock contention is a normal, transient condition. Reacting with a state reset risks wiping healthy sync state whenever two requests overlap.
Changes
State/Sql.php,State/Mongo.php: throwHorde_ActiveSync_Exception_TemporaryFailurewhen the collection lock cannot be acquired. As a subclass ofHorde_ActiveSync_Exception, all existing generic catch sites keep working.Collections.php(pollForChanges()): catchTemporaryFailurebefore the generic handler and skip the collection for this iteration (retried on the next poll) instead of resetting state. The twoStaleStaterecovery resets are wrapped the same way: if the reset is blocked by a held lock, it is deferred to the next iteration instead of aborting the request.State/Base.php(loadState()): readclass/serveridwith null coalescing so an error-recovery reset with an empty collection array creates a plainHorde_ActiveSync_Folder_Collectionwithout PHP 8 warnings (matches pre-PHP-8 behavior).Test plan
TemporaryFailureand rolls back the lock transaction (CollectionLockTest::testHeldLockThrowsTemporaryFailure)loadState([], null, SYNC, $id)emits no warnings and creates a folder object (CollectionLockTest::testLoadStateResetWithEmptyCollectionEmitsNoWarnings)pollForChanges()skips the collection on lock contention without callingloadState()reset (PingCollectionLockContentionTest)